Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

eth/protocols/eth: remove Requests in block body #30562

Merged
merged 2 commits into from
Oct 10, 2024
Merged

eth/protocols/eth: remove Requests in block body #30562

merged 2 commits into from
Oct 10, 2024

Conversation

islishude
Copy link
Contributor

Block no longer has Requests.

It wasn't removed in #30425

@islishude islishude mentioned this pull request Oct 9, 2024
Merged
@fjl fjl changed the title eth/protocols: remove Requests eth/protocols/eth: remove Requests Oct 9, 2024
@fjl fjl changed the title eth/protocols/eth: remove Requests eth/protocols/eth: remove Requests in block body Oct 9, 2024
@MariusVanDerWijden
Copy link
Member

MariusVanDerWijden commented Oct 10, 2024 •
edited
Loading

I think we also need to remove the requestHashes in eth/downloader/downloader_test.go:233

Otherwise the following diff will panic:

diff --git a/eth/downloader/fetchers_concurrent_bodies.go b/eth/downloader/fetchers_concurrent_bodies.go
index 56359b33c9..3d8a1576a0 100644
--- a/eth/downloader/fetchers_concurrent_bodies.go
+++ b/eth/downloader/fetchers_concurrent_bodies.go
@@ -90,6 +90,9 @@ func (q *bodyQueue) request(peer *peerConnection, req *fetchRequest, resCh chan
 func (q *bodyQueue) deliver(peer *peerConnection, packet *eth.Response) (int, error) {
        txs, uncles, withdrawals := packet.Res.(*eth.BlockBodiesResponse).Unpack()
        hashsets := packet.Meta.([][]common.Hash) // {txs hashes, uncle hashes, withdrawal hashes}
+       if len(hashsets) != 3 {
+               panic("adsfadsf")
+       }

@MariusVanDerWijden
Copy link
Member

I think we also need to remove the emptyRequests check from EmptyBody in core/types/block.go.
Otherwise we might try to fetch a block that should not be fetched otherwise, but I'm not 100% sure here cc @lightclient @fjl

@islishude
Copy link
Contributor Author

I think we also need to remove the emptyRequests check from EmptyBody in core/types/block.go.

It was removed in #30425

@MariusVanDerWijden
Copy link
Member

Ah perfect, looks like I was on a wrong branch

MariusVanDerWijden
MariusVanDerWijden approved these changes Oct 10, 2024
View reviewed changes
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fjl fjl merged commit 5b393ac into ethereum:master Oct 10, 2024
3 checks passed
@fjl fjl added this to the 1.14.12 milestone Oct 10, 2024
@islishude islishude deleted the remove-requests branch October 10, 2024 08:51
holiman pushed a commit that referenced this pull request Nov 19, 2024
@islishude @holiman
eth/protocols/eth: remove Requests in block body (#30562)
f1ec974 
Block no longer has Requests. This PR just removes some code that wasn't removed in #30425.
zfy0701 pushed a commit to sentioxyz/go-ethereum that referenced this pull request Dec 3, 2024
@islishude @zfy0701
eth/protocols/eth: remove Requests in block body (ethereum#30562)
ffc6edb 
Block no longer has Requests. This PR just removes some code that wasn't removed in ethereum#30425.
This was referenced Dec 6, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MariusVanDerWijden MariusVanDerWijden MariusVanDerWijden approved these changes

@fjl fjl fjl approved these changes

@karalabe karalabe Awaiting requested review from karalabe

@holiman holiman Awaiting requested review from holiman holiman is a code owner

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.14.12
Development

Successfully merging this pull request may close these issues.
3 participants
@islishude @MariusVanDerWijden @fjl